Skip to content

ext/readline: readline_info fix usage when the buffer is not initialised#15139

Merged
devnexen merged 1 commit into
php:masterfrom
devnexen:ext_readline_upd2
Jul 31, 2024
Merged

ext/readline: readline_info fix usage when the buffer is not initialised#15139
devnexen merged 1 commit into
php:masterfrom
devnexen:ext_readline_upd2

Conversation

@devnexen

Copy link
Copy Markdown
Member

rl_initialise is only called when readline() is used so the global
buffer might not be initialised yet.

@petk

petk commented Jul 28, 2024

Copy link
Copy Markdown
Member

Is this perhaps also related to this issue:

./configure --with-libedit
make 
/php-src/ext/readline/readline.c: In function ‘zif_readline_info’:
/php-src/ext/readline/readline.c:191:41: warning: implicit declaration of function ‘rl_extend_line_buffer’ [-Wimplicit-function-declaration]
  191 |                                         rl_extend_line_buffer(Z_STRLEN_P(value) + 1);
      |                                         ^~~~~~~~~~~~~~~~~~~~~

/usr/bin/ld: ext/readline/readline.o: in function `zif_readline_info':
/php-src/ext/readline/readline.c:191:(.text+0x747): undefined reference to `rl_extend_line_buffer'

The libedit is actually preferred library to use in PHP. See #13184 (readline library shouldn't even be an option to use in PHP).

@devnexen

Copy link
Copy Markdown
Member Author

Not at all related but was not aware of the libedit/libreadline differences.

Comment thread ext/readline/readline.c Outdated
Comment on lines 190 to 195

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am confused, surely having the if (!rl_line_buffer) check first makes more sense? As you then don't need to check for !oldstr ?

@devnexen devnexen force-pushed the ext_readline_upd2 branch from 3ab4ff7 to 3eb00aa Compare July 29, 2024 21:54
devnexen referenced this pull request Jul 30, 2024
if the new value was larger, rl_line_buffer_length was never updated.
neither was rl_end (on unixes).

close GH-15120
@remicollet

Copy link
Copy Markdown
Member

The libedit is actually preferred library to use in PHP. See #13184 (readline library shouldn't even be an option to use in PHP).

Indeed, Licenses are NOT compatible, IMHO at some point we should drop libreadline support

@devnexen

Copy link
Copy Markdown
Member Author

The libedit is actually preferred library to use in PHP. See #13184 (readline library shouldn't even be an option to use in PHP).

Indeed, Licenses are NOT compatible, IMHO at some point we should drop libreadline support

Let s try it for next major release then (php 9 ?).

@remicollet

Copy link
Copy Markdown
Member

Quick check,

If seems rl_end is not updated on Linux with libedit

#if !defined(PHP_WIN32) && !HAVE_LIBEDIT
...
				rl_end = Z_STRLEN_P(value);

But may be returned

#ifndef PHP_WIN32
		} else if (zend_string_equals_literal_ci(what, "end")) {
			RETVAL_LONG(rl_end);
#endif

 rl_initialise is only called when readline() is used so the global
 buffer might not be initialised yet.
@devnexen devnexen force-pushed the ext_readline_upd2 branch from ff2b44f to 56ec48a Compare July 30, 2024 15:29
@andypost

Copy link
Copy Markdown
Contributor

Faced the same trying to build 8.4.0_alpha3

@andypost

Copy link
Copy Markdown
Contributor

Thank you, the PR allowed to pass tests

@remicollet remicollet left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@devnexen devnexen merged commit b7e43bd into php:master Jul 31, 2024
@petk

petk commented Jul 31, 2024

Copy link
Copy Markdown
Member

It works, thanks!

@andypost

andypost commented Jul 31, 2024

Copy link
Copy Markdown
Contributor

Will it cause the alpha3 tarball rebuild or better to wait for beta1?

@devnexen

devnexen commented Aug 1, 2024

Copy link
Copy Markdown
Member Author

Will it cause the alpha3 tarball rebuild or better to wait for beta1?

neither, there is alpha4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants